feat(config): Add 'off' option for failOn and commentOn#19
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
src/action/main.ts
Outdated
| } | ||
|
|
||
| const validSeverities = ['critical', 'high', 'medium', 'low', 'info'] as const; | ||
| const validSeverities = ['off', 'critical', 'high', 'medium', 'low', 'info'] as const; |
There was a problem hiding this comment.
🟠 Potential logic issue: 'off' severity may bypass validation elsewhere
Adding 'off' to the validSeverities array allows it to pass the includes() check on line 66 and be assigned to failOn/commentOn. However, this may cause issues in downstream code that expects only actual severity levels (critical, high, medium, low, info). If 'off' is meant to disable the feature, it should be handled separately (e.g., checking for 'off' and setting the value to undefined), rather than treating it as a valid severity level that gets passed through the system.
Suggested fix: Handle 'off' as a special case that results in undefined, keeping it separate from actual severity levels
| const validSeverities = ['off', 'critical', 'high', 'medium', 'low', 'info'] as const; | |
| const validSeverities = ['critical', 'high', 'medium', 'low', 'info'] as const; | |
| const failOn = failOnInput === 'off' ? undefined : validSeverities.includes(failOnInput as typeof validSeverities[number]) |
warden: security-review
| export function shouldFail(report: SkillReport, failOn: SeverityThreshold): boolean { | ||
| if (failOn === 'off') return false; | ||
| const threshold = SEVERITY_ORDER[failOn]; |
There was a problem hiding this comment.
🟠 Missing null/undefined check before accessing SEVERITY_ORDER
The code accesses SEVERITY_ORDER[failOn] without checking if 'failOn' is a valid key in the SEVERITY_ORDER object. While the early return handles 'off', if an invalid severity value is passed, this could result in 'threshold' being undefined, leading to undefined behavior in the comparison. The same issue exists in the second function accessing SEVERITY_ORDER[f.severity] for each finding.
Suggested fix: Add validation to ensure the severity value exists in SEVERITY_ORDER before accessing it, or add type guards to prevent invalid values from being passed
| export function shouldFail(report: SkillReport, failOn: SeverityThreshold): boolean { | |
| if (failOn === 'off') return false; | |
| const threshold = SEVERITY_ORDER[failOn]; | |
| if (threshold === undefined) return false; |
warden: security-review
| export function filterFindingsBySeverity(findings: Finding[], threshold?: SeverityThreshold): Finding[] { | ||
| if (!threshold) return findings; | ||
| if (threshold === 'off') return []; | ||
| const thresholdOrder = SEVERITY_ORDER[threshold]; | ||
| return findings.filter((f) => SEVERITY_ORDER[f.severity] <= thresholdOrder); |
There was a problem hiding this comment.
🟠 Type mismatch in SEVERITY_ORDER lookup with SeverityThreshold
The function now accepts 'SeverityThreshold' which includes 'off', but line 31 attempts to look up 'threshold' in 'SEVERITY_ORDER' without checking if it's 'off' first. While there is a check for 'off' on line 30, if the order of these checks is changed or if the code is refactored, this could cause a runtime error since 'SEVERITY_ORDER' is typed as 'Record<Severity, number>' and 'off' is not a valid 'Severity'. TypeScript should catch this, but the logic could be more robust.
Suggested fix: Add explicit type narrowing or use type guards to ensure 'threshold' is a valid Severity before SEVERITY_ORDER lookup. Alternatively, restructure the checks to make the intent clearer.
| export function filterFindingsBySeverity(findings: Finding[], threshold?: SeverityThreshold): Finding[] { | |
| if (!threshold) return findings; | |
| if (threshold === 'off') return []; | |
| const thresholdOrder = SEVERITY_ORDER[threshold]; | |
| return findings.filter((f) => SEVERITY_ORDER[f.severity] <= thresholdOrder); | |
| export function filterFindingsBySeverity(findings: Finding[], threshold?: SeverityThreshold): Finding[] { | |
| if (!threshold) return findings; | |
| if (threshold === 'off') return []; | |
| // At this point, threshold is guaranteed to be a valid Severity | |
| const thresholdOrder = SEVERITY_ORDER[threshold as Severity]; |
warden: security-review
src/action/main.ts
Outdated
| } | ||
|
|
||
| const validSeverities = ['critical', 'high', 'medium', 'low', 'info'] as const; | ||
| const validSeverities = ['off', 'critical', 'high', 'medium', 'low', 'info'] as const; |
There was a problem hiding this comment.
🟠 Addition of 'off' severity may bypass security controls
The 'off' value added to validSeverities allows users to disable security checks via fail-on or comment-on inputs. While this may be intentional for flexibility, it creates a risk that security findings will be ignored. If 'fail-on' is set to 'off', the action will never fail on security findings regardless of their severity. This could allow critical vulnerabilities to pass through CI/CD pipelines unnoticed if misconfigured or intentionally disabled by malicious actors with repository write access.
Suggested fix: Consider: 1) Document this behavior clearly in user-facing documentation with security warnings, 2) Log a warning when 'off' is used to ensure visibility, 3) Consider whether 'off' should only be allowed for comment-on but not fail-on to maintain minimum security guardrails, or 4) Require explicit acknowledgment (e.g., 'disabled' instead of 'off') to prevent accidental misconfiguration.
| const validSeverities = ['off', 'critical', 'high', 'medium', 'low', 'info'] as const; | |
| if (failOnInput === 'off') { | |
| console.warn('WARNING: fail-on is set to "off" - security findings will not fail the build'); | |
| } | |
warden: security-review
| export function shouldFail(report: SkillReport, failOn: SeverityThreshold): boolean { | ||
| if (failOn === 'off') return false; | ||
| const threshold = SEVERITY_ORDER[failOn]; |
There was a problem hiding this comment.
🟠 Potential type confusion between Severity and SeverityThreshold
The functions now accept 'SeverityThreshold' (which presumably includes 'off') but use SEVERITY_ORDER lookup without verifying that 'off' is excluded from the lookup. If 'off' is not defined in SEVERITY_ORDER, the threshold variable will be undefined, which could lead to unexpected behavior in the comparison operations. While there's an early return for 'off', the type system now allows 'off' to reach SEVERITY_ORDER[failOn] if the check is removed or bypassed.
Suggested fix: Consider using a type guard or assertion after the 'off' check to ensure type safety, or ensure SEVERITY_ORDER explicitly handles all SeverityThreshold values including 'off'.
| export function shouldFail(report: SkillReport, failOn: SeverityThreshold): boolean { | |
| if (failOn === 'off') return false; | |
| const threshold = SEVERITY_ORDER[failOn]; | |
| // Type assertion to ensure failOn is now Severity after 'off' check | |
| const severityLevel: Severity = failOn as Severity; | |
| const threshold = SEVERITY_ORDER[severityLevel]; |
warden: security-review
| if (!threshold) return findings; | ||
| if (threshold === 'off') return []; | ||
| const thresholdOrder = SEVERITY_ORDER[threshold]; |
There was a problem hiding this comment.
🟠 Type mismatch when accessing SEVERITY_ORDER with SeverityThreshold
The function now accepts SeverityThreshold which includes 'off', but SEVERITY_ORDER is typed as Record<Severity, number> which does not include 'off'. When threshold is not 'off', the code accesses SEVERITY_ORDER[threshold] which could be 'off' at the type level, causing a type error. The runtime check for 'off' happens before this access, but TypeScript may not narrow the type correctly, potentially allowing undefined to be assigned to thresholdOrder.
Suggested fix: Cast threshold to Severity after the 'off' check, or use type guard to narrow the type
| if (!threshold) return findings; | |
| if (threshold === 'off') return []; | |
| const thresholdOrder = SEVERITY_ORDER[threshold]; | |
| const thresholdOrder = SEVERITY_ORDER[threshold as Severity]; |
warden: security-review
security-reviewsecurity-review: No issues found No findings to report. |
security-reviewsecurity-review: No issues found No findings to report. |
Allow users to completely disable failure checks and PR comments by setting failOn and/or commentOn to 'off'. - failOn=off: Never fail the check, regardless of findings - commentOn=off: Never post PR comments/annotations Introduces SeverityThreshold type that extends Severity with 'off', keeping the existing Severity type unchanged for findings. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When commentOn was set to 'off', a summary comment "No findings to report" was still being posted because postReviewToGitHub was called unconditionally. Now the commentOn value is tracked in TriggerResult and checked before posting. Also moves misplaced import statement to top of output/types.ts. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Instead of computing renderResult and then checking commentOn before posting, skip the renderSkillReport call entirely when comments are disabled. This removes unnecessary work and simplifies TriggerResult. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
0d442c5 to
c8945d6
Compare
Replace duplicated inline union types and validSeverities array with imports from src/types/index.ts. Uses SeverityThresholdSchema.safeParse for validation instead of manual array inclusion check. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
c8945d6 to
005cf97
Compare
| export function filterFindingsBySeverity(findings: Finding[], threshold?: Severity): Finding[] { | ||
| export function filterFindingsBySeverity(findings: Finding[], threshold?: SeverityThreshold): Finding[] { | ||
| if (!threshold) return findings; | ||
| if (threshold === 'off') return []; |
There was a problem hiding this comment.
🟠 Type mismatch when accessing SEVERITY_ORDER with SeverityThreshold
The function now accepts SeverityThreshold which includes 'off', but SEVERITY_ORDER is typed as Record<Severity, number> which does not include 'off'. While the code handles 'off' before accessing SEVERITY_ORDER[threshold], TypeScript's type system will flag this as an error because threshold is still typed as SeverityThreshold (not narrowed to Severity) when used as the key.
Suggested fix: Add a type assertion or use a type guard to narrow the threshold type after the 'off' check, making the type relationship explicit.
| if (threshold === 'off') return []; | |
| const thresholdOrder = SEVERITY_ORDER[threshold as Severity]; |
warden: find-bugs
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| groupByFile?: boolean; | ||
| /** Only include findings at or above this severity level in rendered output */ | ||
| commentOn?: 'critical' | 'high' | 'medium' | 'low' | 'info'; | ||
| extraLabels?: string[]; |
There was a problem hiding this comment.


Allow users to completely disable failure checks and PR comments by setting
failOnand/orcommentOnto"off".failOn=off: Never fail the check, regardless of findingscommentOn=off: Never post PR comments/annotationsThis is useful when users want to run Warden for informational purposes only without affecting CI status or creating PR noise.
Introduces a
SeverityThresholdtype that extendsSeveritywith'off', keeping the existingSeveritytype unchanged for findings themselves.Usage:
In
warden.toml:Via CLI:
Via GitHub Action: